Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add delete flag to external-resources #4521

Merged
merged 1 commit into from
Jul 8, 2024

Conversation

lechuk47
Copy link
Contributor

@lechuk47 lechuk47 commented Jul 2, 2024

External resources deletion will work with the delete: true attribute at the resource level.

externalResources:
- provider: aws
  provisioner:
    $ref: /aws/app-sre-stage/account.yml
  resources:
  - provider: rds
    managed_by_erv2: true
    identifier: erv2-tests
    defaults: /terraform/resources/erv2-tests/rds/pg14-defaults.yml
    parameter_group: /terraform/resources/erv2-tests/rds/pg14-default-pg.yml
    delete: true # <-- This resource will get deleted.

Why:

  • This is a well-established pattern to delete resources in app-interface (saas targets, namespaces, etc).
  • TF-Resources deletion_approvals are meant to avoid non-wanted resource deletions in case resource specs are removed from app-interface. Requiring a delete: true attribute accomplishes the same objective.
  • Tenants don't need to care about secondary terraform objects to delete, they just want to remove the resource.
  • This change introduces the "feature" of abandoning resources.

Schema update: app-sre/qontract-schemas#683

reconcile/external_resources/manager.py Outdated Show resolved Hide resolved
Comment on lines 316 to 318
resource.dict(
exclude={"data": {FLAG_RESOURCE_MANAGED_BY_ERV2, FLAG_DELETE_RESOURCE}}
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to exclude those fields? There is a similar method serialize_input in ExternalResource, and to serialize json, can directly call resource.json( instead of resource.dict( then json.dumps

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • These fields are not expected in the module input. Since the overrides parameter doesn't have a schema, all the overridden parameters are passed to the terraform object directly (the same way as terraform-resources). So all parameters not expected in the input shall be removed.

  • I changed the call to resource.json() and removed the method in ExternalResource (this was used in an old implementation when the INPUT was passed as an env var)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to ignore unexpected input or define a clear mapping? Otherwise we have to remember edit this part whenever we put new fields in data

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cannot think of anything to do that as we want the ability to add additional fields in the overrides.
There should not be a lot of changes here since only "logical" parameters need to be introduced.
I agree though, I'll try to overcome this or at least have a way to detect it with a test or something.

Copy link
Contributor

@hemslo hemslo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@lechuk47 lechuk47 merged commit c147740 into app-sre:master Jul 8, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants